gh-149728: Fix free-threaded race in importlib lazy-submodule fast path#149729
gh-149728: Fix free-threaded race in importlib lazy-submodule fast path#149729SwayamInSync wants to merge 3 commits into
Conversation
SwayamInSync
requested review from
FFY00,
brettcannon,
ericsnowcurrently,
ncoghlan and
warsaw
as code owners
|
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
|
All commit authors signed the Contributor License Agreement. |
| @@ -0,0 +1,7 @@ | |||
| Fix a race in :mod:`importlib._bootstrap` where the fast path in | |||
There was a problem hiding this comment.
no need to say the internal details, something like "fix race in importlib when importing module which defined getattr" should be enough
Fixes a race condition in importlib's module loading when using lazy submodules, preventing RecursionError in multi-threaded scenarios.
| @@ -926,8 +927,10 @@ def _load_unlocked(spec): | |||
| module = sys.modules.pop(spec.name) | |||
| sys.modules[spec.name] = module | |||
| _verbose_message('import {!r} # {!r}', spec.name, spec.loader) | |||
There was a problem hiding this comment.
I may not be the best person to review this, but sprinkling the spec._initializing = False calls through out seems a little fragile. I would suggest doing something like adding a finish_load = None parameter to _load_unlocked and if it's not None then you call it here after all of the the other initialization has succeeded.
Most of the call sites then don't change, but _find_and_load_unlocked and then move the parent module initialization into a nested function and pass that in.
4 participants
Fixes #149728.
Cause
_load_unlockedclearsspec._initializingat the end of its body, before_find_and_load_unlockedrunssetattr(parent_module, child, module). Between those two events,sys.modules[name]is set and_initializing == False, butparent.__dict__[child]is still missing. The fast path in_find_and_loadreturns the module without taking the import lock once it sees_initializing == False, so under free-threaded CPython a second thread can observe this window.IMPORT_FROM 'child'on that thread doesgetattr(parent, 'child'), falls into a lazy__getattr__, runs the sameimport parent.child as childline, fast-paths again, and recurses toRecursionError. See the linked issue for the full walkthrough and reproducer.Change
Keep
spec._initializing == Trueuntil after the parent setattr in_find_and_load_unlocked:_load_unlockedno longer clears_initializingon the success path. Failure paths still clear it._find_and_load_unlockedclears_initializingin afinallyblock aftersetattr(parent_module, child, module)and_imp._set_lazy_attributes._load_unlocked(_loadand_builtin_from_name) have no parent setattr, so they clear_initializingin a localfinally.This preserves the invariant the fast path needs:
_initializing == Falseimplies the module is reachable viagetattr(parent_module, child).Test
Lib/test/test_importlib/test_threaded_import.py::ThreadedImportTests::test_lazy_submodule_getattr_no_recursionwidens the natural microsecond race window with onethreading.Eventand verifies that an observer thread does not recurse on the lazy__getattr__. Fails on the unpatched interpreter, passes on this branch. Fulltest_importlibsuite remains green (1220/1220).Notes
__getattr__#149728